Open
Conversation
joe-redpanda
force-pushed
the
constants
branch
from
5c640e3 to
5a989b7
Compare
joe-redpanda
force-pushed
the
constants
branch
from
5a989b7 to
6b5113b
Compare
Contributor
Author
|
rebase |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new constants module to centralize commonly used magic numbers across the Redpanda codebase. The change replaces hardcoded concurrency limits (32) and a partition balancer constant (7) with named constants defined in new header files.
Changes:
- Created
constants::common::default_concurrencyto replace hardcoded32values used in concurrent operation limits - Created
constants::balancer::missed_statuses_until_unresponsiveto replace hardcoded7in partition balancer logic - Updated all usages across multiple modules to reference these constants
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/v/constants/common.h | Defines default_concurrency constant (32) for concurrent operations |
| src/v/constants/balancer_constants.h | Defines missed_statuses_until_unresponsive constant (7) for partition balancer |
| src/v/constants/BUILD | Bazel build definitions for the new constants libraries |
| src/v/redpanda/admin/partition.cc | Uses default_concurrency in max_concurrent_for_each call |
| src/v/kafka/server/handlers/describe_transactions.cc | Uses default_concurrency for transaction description concurrency |
| src/v/kafka/data/rpc/service.cc | Replaces hardcoded concurrency limit with constant |
| src/v/datalake/translation/scheduling.cc | Uses default_concurrency in translator cleanup |
| src/v/datalake/translation/tests/scheduler_fixture.h | Uses constant for test fixture configuration |
| src/v/cluster_link/replication/link_replication_mgr.h | Uses constant for semaphore initialization |
| src/v/cluster_link/group_mirroring_task.h | Uses constant for concurrent request limit |
| src/v/cluster/topics_frontend.cc | Uses default_concurrency for partition move cancellation |
| src/v/cluster/rm_stm.cc | Uses constant in multiple producer management operations |
| src/v/cluster/partition_balancer_backend.cc | Uses both concurrency and balancer constants |
| src/v/config/validators.cc | Uses balancer constant in validation logic and error messages |
| Multiple BUILD files | Add dependencies on new constants libraries |
| public: | ||
| // The partition balancer will declare a node unresponsive if it misses this | ||
| // many node statuses in a row | ||
| static constexpr uint8_t missed_statuses_until_unresponsive = 7u; |
There was a problem hiding this comment.
The type uint8_t is unnecessarily restrictive for a counter value. Consider using size_t or int for consistency with typical integer constants and to avoid potential overflow issues if the value needs to increase in the future.
Contributor
Author
There was a problem hiding this comment.
If I want more than 255 I'll just change the type : )
Adds a bazel module for capturing codebase-wide constants called common_constants. To this, adds a constant called "default_concurrency" set to 32. This value is wiely used as a magic number in the cluster. A future commit will replace usages of the magic number with this constant
Replaces all usages of the magic number 32 with the common constant 'default_concurrency'
Adds balancer constants to encapsulate the constants used in the various balancers (leader and partition). Extracts a constant to represent the number of node statuses that may be missed before the balancer considers a node unresponsive (7). Uses that constant in the partition balancer planner
Use the balancer constant rather than a magic number
joe-redpanda
force-pushed
the
constants
branch
from
6b5113b to
52cf3e1
Compare
Contributor
Author
|
[23,557 / 23,559] 895 / 900 tests; Testing //src/v/cloud_topics/level_one/domain/tests:db_domain_manager_test; 1200s remote-cache, linux-sandbox ... (2 actions running) Probably unrelated but will take a peek |
Contributor
Author
|
Contributor
Author
|
bazel test failure was unrelated, requesting feedback |
joe-redpanda
requested review from
bashtanov,
bharathv,
dotnwat and
mmaslankaprv
Contributor
Author
|
The question: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
"Why?"
This was prompted by the magic number '7' in partition balancer.
I added validation on the relative sizes of time spans in the partition balancer when updating configs.
Generally node status < node unresponsiveness < node drain < auto decommission
Problem: node unresponsiveness is defined as 7 * node status interval
This number lives (lived) hardcoded in partition_balancer_backend
Should this multiplier be a configuration?
maybe?
Is it okay to have hardcoded constants?
IMO yes, just so long as theres ONE definition.
So, to have one definition usable by config validation and partition balancer, I can
I scraped together the usages of 32 as well mostly as a demonstration that this folder can be more widely useful than just partition balancer.
Backports Required
Release Notes